Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(cxl-ui): add cxl-search-filters and related components #297

Open
wants to merge 21 commits into
base: feat/dashboard
Choose a base branch
from
Open

feat(cxl-ui): add cxl-search-filters and related components #297

wants to merge 21 commits into from

Conversation

freudFlintstone
Copy link

Task: https://app.clickup.com/t/861mzgnx5

These changes depend on being able to replace some of the elements rendered by the FacetWP plugin. Here's a gist for a similar problem:
https://gist.github.com/MWDelaney/7f3ff797041bac5ef9918112d6a43816

@freudFlintstone freudFlintstone self-assigned this Jul 14, 2023
@freudFlintstone freudFlintstone force-pushed the raphael/feat/dashboard/institute-page-filter-pane branch from 6c36711 to e5fe8f6 Compare July 25, 2023 21:37
@freudFlintstone freudFlintstone force-pushed the raphael/feat/dashboard/institute-page-filter-pane branch 3 times, most recently from d1b7a38 to be528c3 Compare July 28, 2023 01:54
@freudFlintstone
Copy link
Author

A few notes:

  • The stories are meant to showcase the design compliance, but can't properly reproduce the expected behavior when used in production.
  • needs to be tested using wp-starter with a catalog page example (added here)
  • The search box and sort dropdown are only placeholders and their styles should not be taken into account. The goal here is to test the responsive layout as proposed in figma
  • The correct vaadin elements to be used resulted in conflicting dependencies in the storybook package, even when trying to set the exact package version. Maybe @anoblet can help me with that. Nevertheless, those elements are slotted in, so using the right ones it would not interfere with the layout work that's already done, although it would obviously be nice to test storybook.
  • This is not necessarily final. We decided to start the review process to help figure out the particulars of integrating with FacetWP

@freudFlintstone freudFlintstone marked this pull request as ready for review July 28, 2023 01:55
@pawelkmpt
Copy link

For easier review but also tracking code it'd be better to have every component as separate commit.

pawelkmpt
pawelkmpt reviewed Jul 28, 2023
View reviewed changes
Copy link

@pawelkmpt pawelkmpt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good at the first glance. We'll test it with PHP layer next week.

I'd be happy to get feedback from @anoblet and @lkraav too.

packages/cxl-ui/src/components/cxl-filter-header.js
import cxlDashboardFilterHeaderStyles from '../styles/cxl-filter-header-css.js';

const supportsScrollEndEvent = 'onscrollend' in window
const isFirefox = document.scrollingElement.scrollLeftMax !== undefined
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's with Firefox that we need special flag?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When in smaller screens, I implemented scrolling for the tabs, like in the design. I also implemented scroll snapping, so that it aligns to the start of the closest tab when the user stops scrolling. In Firefox, the smooth scrolling behavior causes a bug that prevents it from scrolling further on very specific screen sizes and tab widths, so I'm just using that to disable smooth scrolling in that case

packages/cxl-ui/src/components/cxl-filter-header.js Show resolved Hide resolved
packages/cxl-ui/src/components/cxl-filter-header.js
render () {
return html`
<div class="container">
<div class="tabs">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about using https://vaadin.com/docs/latest/components/tabs here @freudFlintstone? Beneficial or overkill?

Copy link
Author

@freudFlintstone freudFlintstone Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried, really hard... but I wasted a day on them, it was not looking as good as the design, and in the end it would actually not work in this specific case, because of how FacetWP works. Also, overkill, since we only need the looks and basically none of the functionality.

packages/cxl-ui/src/components/cxl-search-filters-item.js Show resolved Hide resolved
packages/cxl-ui/src/components/cxl-search-filters-item.js Outdated Show resolved Hide resolved
@freudFlintstone freudFlintstone force-pushed the raphael/feat/dashboard/institute-page-filter-pane branch 2 times, most recently from 3fdb5e5 to 413afce Compare July 28, 2023 10:39
@freudFlintstone
Copy link
Author

For easier review but also tracking code it'd be better to have every component as separate commit.

Do you want me to go back and make separate commits? It's doable

@pawelkmpt
Copy link

For easier review but also tracking code it'd be better to have every component as separate commit.

Do you want me to go back and make separate commits? It's doable

Yes, please

@freudFlintstone freudFlintstone force-pushed the raphael/feat/dashboard/institute-page-filter-pane branch from 413afce to ea23944 Compare August 1, 2023 14:14
@anoblet
Copy link
Collaborator

anoblet commented Aug 7, 2023

Would it make sense to default the first tab to selected?

image

@freudFlintstone
Copy link
Author

Would it make sense to default the first tab to selected?

image

Yes. I'll update the story to correctly depict that intention, but it really depends on how FacetWP sets the default on the back-end.

@freudFlintstone freudFlintstone force-pushed the raphael/feat/dashboard/institute-page-filter-pane branch from ea23944 to bb559b1 Compare August 8, 2023 21:30
@kertuilves kertuilves force-pushed the feat/dashboard branch 2 times, most recently from 3e83479 to 7c4b890 Compare August 17, 2023 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lkraav lkraav Awaiting requested review from lkraav

@anoblet anoblet Awaiting requested review from anoblet

@pawelkmpt pawelkmpt Awaiting requested review from pawelkmpt

Assignees

@freudFlintstone freudFlintstone

Labels
CXL 2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@freudFlintstone @pawelkmpt @anoblet @phkel @kertuilves